IBX-6773: Bookmarks for non accessible contents cause exception #408
Conversation
vidarl
force-pushed
the
IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception
branch
2 times, most recently
from
a15afa0
to
7246b2e
Compare
vidarl
force-pushed
the
IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception
branch
from
e2d1c9a
to
ddca556
Compare
eZ/Publish/API/Repository/Values/Content/Query/Criterion/IsBookmarked.php
Outdated
Show resolved
Hide resolved
.../Persistence/Legacy/Tests/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php
Outdated
Show resolved
Hide resolved
eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php
Outdated
Show resolved
Hide resolved
eZ/Publish/API/Repository/Values/Content/Query/SortClause/BookmarkId.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Paweł Niedzielski <[email protected]>
…n - removed covers
…n - Changed to ibexa namespace for new classes
…xception - Changed to ibexa namespace for new classes
…xception - Changed to ibexa namespace for new classes
…xception - changed deprecations messages
Steveb-p
changed the title
| $expectedIdsAfter = array_map(static function (Location $item) use ($demoDesignLocationId, $contactUsLocationId) { | ||
| if ($item->id === $demoDesignLocationId) { | ||
| return $contactUsLocationId; | ||
| } | ||
|
|
||
| return $item->id; | ||
| }, $beforeSwap->items); |
There was a problem hiding this comment.
Why is this mapping necessary? In particular, why is there a special case for $demoDesignLocationId + $contactUsLocationId?
There was a problem hiding this comment.
The mapping is no longer needed. In first version of PR, I did not implement SortClause\BookmarkId, so order of the result needed to be normalized.
I will revert the mapping.
I think the test is better when adding $contactUsLocation. In original test we test the edge case where two bookmarked locations are swapped... In my version of the test, only one of the locations that are swapped are bookmarked.
Edit: Fixed cut&paste error that made the second paragraph meaningless
|
| } catch (BadStateException $e) { | ||
| return new BookmarkList(); | ||
| } |
There was a problem hiding this comment.
This exception should probably be logged. We had plenty of situations in the past where people asked why something isn't working as it should, while we "swallowed" the exception.
v3.3If user bookmark some location which he later looses access too, then the bookmark list in admin-ui fails with an exception.
Simply fixing
BookmarkService::loadBookmarks()would be easy. The problem is to implementcountUserBookmarks()in persistence layer and having it taking into account user permissions so that BC would be kept.Talked with Adam on how to solve this without breaking BC and he suggested implementing it using filtering
The Bookmark filter will only work with location filtering (
LocationService::find()), not with content (ContentService::find())Checklist:
$ composer fix-cs).@ezsystems/engineering-team).